Repository navigation
fix(auth): contain destructive identity consolidation - #7459
Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — app-only #6827 identity-consolidation containment.
This PR hard-disables identity consolidation (mergeUsers, promoteSecondaryIfPrimaryDeleted) and replaces the user.deleted promotion path with a non-promoting deleteIdentityCredential, fail-closed at every write seam and thoroughly tested. No protocol surface (static/schemas/source/**, normative docs/reference/**, dist artifacts) is touched, so the absent changeset is correct. No critical or high findings.
Medium findings
- server/src/routes/workos-webhooks.ts (deletion path) — the new non-promoting deletion can leave a historical multi-credential identity with zero primaries and emits no ops signal, so a surviving secondary can hit the #3718 empty-workspace DoS silently.
Decision path: no critical/high findings (row 1 not triggered). gated_paths is false (row 2 N/A). high_risk is true, but the only high-risk-path change with a change-kind of (added) is the new identity-mutation-containment.ts — new files are normal scaffolding — and the sole medium finding is on workos-webhooks.ts (modified). Wait: row 5 fires when high_risk is true AND a (modified) high-risk file exists AND any medium finding is present.
Re-evaluating row 5: workos-webhooks.ts (modified) matches a high-risk glob and there is one medium finding present — this satisfies row 5. Outcome is escalate, not approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean delta, no blocking findings.
This PR extends deleteIdentityCredential with post-commit orphan detection (surviving credentials but no primary) that warns and fires a throttled, fire-and-forget notifySystemError, backed by thorough integration and unit coverage. The reviewer verified branch logic, transaction/rollback semantics, post-commit-after-release ordering, and confirmed the notification carries no PII — all correct and tested.
No changes to static/schemas/source/**, docs/reference/**, or dist/** artifacts, so schema↔docs coherence, changeset scope, oneOf discriminator, and artifact-immutability gates do not apply. This is a server-only identity/auth-plumbing change.
Decision-table walk:
- Rows 1: no critical/high findings.
- Row 2: gated_paths is false — not triggered.
- Row 3: high_risk true, but no
(deleted)entries. - Row 4: no medium findings at all.
- Row 5: high_risk (modified) present, but no medium findings.
- Row 6: prior decision was
approve, notescalate. - Row 7: no no-auto-approve team match.
- Row 8: fewer than three medium findings (zero).
- Row 9: approve.
The high_risk flag reflects modifications under server/src/routes/** plus a newly added route file; per the flag-reading guidance, a modification with no medium-or-higher concern is presumed safe, and new files are normal scaffolding. Consistent with the prior clean approve on this PR.
8dcb7d5 to
10762a0
Compare
10762a0 to
3cd1575
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
3cd1575 to
e323c3d
Compare
e323c3d to
9a7cd53
Compare
2da9a75 to
4120a39
Compare
6e35597 to
c5873d6
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — server-only identity-containment PR, no blocking or medium findings.
This is a subsequent pass (prior outcome: approve, no findings). The reviewer reports the current delta is a pure rebase onto main with a byte-identical patch. Re-reading the full production surface (identity-db confirmed-deletion transaction, auth.ts fail-closed lifecycle reads, oauth/http/webhooks credential-event guards, backfill circuit breaker) surfaced no new Critical/High/Medium findings; design is fail-closed and internally consistent.
Decision-table walk:
- Row 1 (critical/high): no findings — skip.
- Row 2 (gated_paths): gated_paths is false — skip.
- Row 3 (high_risk deletion): high_risk is true but all high_risk_reasons are (modified)/(added), no (deleted) — skip.
- Row 4 (medium data-loss/schema/infra): no medium findings — skip.
- Row 5 (high_risk modified + medium): no medium findings — skip.
- Row 6 (sticky escalate): prior was approve, not escalate — skip.
- Row 7 (no-auto-approve team): no team match — skip.
- Row 8 (≥3 medium): zero medium — skip.
- Row 9: approve.
No schema/dist/task/normative-doc surface touched, so no changeset required. review_decision is REVIEW_REQUIRED, but no hard gate applies (gated_paths false, no no-auto-approve team, no breaking-class protocol change), so approve stands at row 9.
c5873d6 to
9fa7707
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass, no blocking findings.
This is a server-only change (server/**) to identity/auth containment surfaces. The reviewer inspected the load-bearing paths directly: identity-db.ts deletion transaction (sorted advisory locks, 1s lock_timeout, exact-cardinality revocation guards, tombstone replay idempotency), auth.ts fail-closed read-path fences, authorization-epoch-db.ts, and workos-webhooks.ts routing. Operational readiness holds (bounded WorkOS prefetch, capped lock-error retries, backfill circuit breaker). No schema/dist/changeset surface touched, so no changeset required. The delta since the prior approve is test-only.
Decision-table walk:
- Row 1 (critical/high): no such findings.
- Row 2 (gated_paths + not approved): gated_paths is false — does not fire.
- Row 3 (deleted high-risk path): high-risk reasons are (modified)/(added) only, no deletions.
- Row 4 (medium data-loss/schema/infra): no medium findings.
- Row 5 (modified high-risk + medium): no medium findings.
- Row 6 (sticky escalate): prior decision was approve, not escalate.
- Row 7 (no-auto-approve team): no team match.
- Row 8 (≥3 medium): zero medium findings.
- Row 9: approve.
The high_risk flag reflects modifications under server/src/routes/**, but the flag alone is a heuristic to look harder, not an escalation trigger. The reviewer found no medium-or-higher concern on those modified files, so the modifications are presumed safe. review_decision: REVIEW_REQUIRED does not force any gate here because gated_paths is false and no prose hard gate (spec/schema/breaking-change governance) applies to this server-only diff.
Contain destructive generic identity consolidation while retaining the authenticated-credential administrator authority landed in #7450 and #7452. This remains containment for #6827, not closure. Generic consolidation and automatic replacement-primary promotion remain refused.
Confirmed provider deletion uses the existing durable audit store, sorted credential/identity locks, exact-cardinality revocation, and a terminal lifecycle recheck. Missing-primary survivors require manual recovery; no replacement primary is inferred. Cookie, Bearer, and MCP admission/cache hits require a current primary-database credential lifecycle snapshot. Provider writes without a durable intent journal remain deferred. Local transactional rollback and next-hit lifecycle denial do not guarantee provider/local consistency, distributed cache pub/sub, or cancellation of already admitted requests.
Replacement qualification
cfc33f6448a6e0ded4afd71c72d501398c346cdb. Initially verified basec9a8c4f91b056aea6871c76c80e9afe307971ffais the fix(auth): authorize web admins by authenticated credential #7452 squash with parentf928b36ff4f721ae64c73cc5f80de65b3d69a071. Main advanced once during the mandatory hook through fix(protocol): align 3.2 error recovery compatibility #7570; its 17 paths have zero PR overlap and were independently reviewed.9fa7707c828be7d75dd1c41b12a1549121f1010e; final tree and independently computed merge tree:74e47c4e36fa58fc5d87a0d120d91491523c2d8a.c5873d64b1a3d20833671618aa178c250cf8dee4. Published under this explicit force-with-lease after rechecking the unchanged qualified base, original remote head, Draft state, auto-merge off, and clean exact candidate. Normal pre-push version-sync and changeset-scope checks passed; the normal hook correctly found no storyboard or documentation scope.core.abbrev=10): 425,444 bytes, SHA-25606a7f07163f65cbc07c0a66122315f9e50f49be5a7694a54964af16261e5e73f. Binary/full-index diff: 428,024 bytes, SHA-25663f05c44007d95ed37b5dd5aebd6f451b095996177a314dd98b66b21e57a7a44. Aggregate stable patch ID:c276c0b8621f58e4fa0b05c74e1edc1a4a551456(original:6dcd3cbfcff77f1c9ed21cea073a1166d11608e4).Explicit compatibility delta
Nine paths overlap upstream: the two Addie admin lookup modules; working-group DB; HTTP; auth middleware; and the aao-admin-status, addie-admin-global-auth, network-health-security, and working-group-admin-global-auth test files.
The parent's two production admin lookup modules are retained byte-for-byte. Main removes both positive and negative authorization caches and preserves typed outage handling; it subsumes the child's older positive-cache removal. The other production overlaps retain both parent exact-credential/reserved-admin boundaries and child lifecycle fences. All non-overlap production blobs remain identical to the reviewed child.
The textual checkpoint exposed two tests calling cache getters removed by #7450. The only subsequent deliberate delta is three test files, +52/−5, folded into original commit 5: replace the removed web cache assertion with observable same-module grant-to-revocation behavior; remove the obsolete Slack cache getter assertion while retaining live DB before/after authorization, quarantine, and replay assertions; add four cold/warm route regressions for oppositely privileged linked credentials and terminal/missing-primary administrator sessions. No production containment is weakened, and no new migration, dependency, changeset, or protocol delta is introduced by this PR.
Local validation and review
be6336113c9b2db393b6c3a0550bc72f727c9955equal main. No PR allocation. Addie tool reference/inventory and portability checks pass (249 tools, 69 sets).Initial published-head CI qualification
Sixteen checks passed before Ready, including both database migration smoke tests, duplicate allocations, Dependency Review, Semgrep, GitGuardian, web build/tests, and three canonical shards. The exact base-to-head dependency-graph comparison is empty (
[]). Semgrep ran five rules on 860 files with zero findings; it reports 39 partially analyzed files, the same partial-file count reported on the qualified base. This scanner coverage limit is not described as complete coverage. Remaining GitHub jobs and exact-head Secretariat review are still required.Terminal published-head qualification
105208047183, on the same exact head. Ready-state Ladon run 35223552620, job105209353413, succeeded and supplied aao-secretariat APPROVED on9fa7707c828be7d75dd1c41b12a1549121f1010eat2026-09-17T12:57:55Z, with an empty findings array. The user explicitly accepts this exact-head approval as the human-review-equivalent gate.74e47c4e36fa58fc5d87a0d120d91491523c2d8a, with parentscfc33f6448a6e0ded4afd71c72d501398c346cdband9fa7707c828be7d75dd1c41b12a1549121f1010e. There are zero unresolved review threads. Ready state and auto-merge off were verified.Immediately before squash merge, recheck the unchanged live base/head, complete check rollup, exact approval, mergeability/tree, and thread state. Use the exact-head match without admin bypass or explicit branch deletion. This remains containment for #6827; generic consolidation and automatic primary promotion remain refused. Later PRs remain untouched.